add migrate_cri #1519

zt1046656665 · 2022-09-22T10:25:24Z

What type of PR is this?

migrate

What this PR does / why we need it:

we add migrate_cri

Which issue(s) this PR fixes:

Fixes #

Special notes for reviewers:

Does this PR introduced a user-facing change?

Additional documentation, usage docs, etc.:

zhouqiu0103 · 2022-09-23T02:09:08Z

Could you please provide documentation or description of the newly added features? Such as the purpose of the command, examples of command usage.The documentation can be referenced to commands docs

zt1046656665 · 2022-09-23T02:31:39Z

Could you please provide documentation or description of the newly added features? Such as the purpose of the command, examples of command usage.The documentation can be referenced to commands docs

ok, i will add it soon,thanks

zhouqiu0103 · 2022-09-23T02:24:43Z

pkg/pipelines/migrate_container.go

+		loaderType = common.AllInOne
+	}
+
+	fmt.Println("MigrateCriPipeline  called")


Why print the information in this place？

zhouqiu0103 · 2022-09-23T02:54:48Z

pkg/container/module.go

+	if p.KubeConf.Arg.Type == common.Docker && p.KubeConf.Arg.Role == common.Worker {
+		p.Tasks = MigrateToDocker(p)
+	} else if p.KubeConf.Arg.Type == common.Docker && p.KubeConf.Arg.Role == common.Master {
+		p.Tasks = MigrateMToDocker(p)
+	} else if p.KubeConf.Arg.Type == common.Conatinerd && p.KubeConf.Arg.Role == common.Worker {
+		p.Tasks = MigrateToContainerd(p)
+	} else if p.KubeConf.Arg.Type == common.Conatinerd && p.KubeConf.Arg.Role == common.Master {
+		p.Tasks = MigrateMToContainerd(p)


Recommended use：

if p.KubeConf.Arg.Type == common.Docker { if p.KubeConf.Arg.Role == common.Worker { // xxx } if p.KubeConf.Arg.Role == common.Master { // xxx } } if p.KubeConf.Arg.Type == common.Conatinerd { if p.KubeConf.Arg.Role == common.Worker { // xxx } if p.KubeConf.Arg.Role == common.Master { // xxx } }

Yes, it is better, I will optimize it, thank you

24sama

Thanks for your contributions! There are some comments, PTAL.

24sama · 2022-09-23T02:03:12Z

cmd/ctl/cri/cri.go

@@ -0,0 +1,46 @@
+/*
+Copyright 2020 The KubeSphere Authors.


2020 -> 2022

24sama · 2022-09-23T02:05:24Z

cmd/ctl/cri/migrate_cri.go

+	cmd.Flags().StringVarP(&o.Role, "role", "", "worker", "Role to migrate")
+	cmd.Flags().StringVarP(&o.Type, "type", "", "docker", "Type to migrate")
+	cmd.Flags().StringVarP(&o.ClusterCfgFile, "filename", "f", "", "Path to a configuration file")
+	cmd.Flags().StringVarP(&o.Kubernetes, "with-kubernetes", "", "", "Specify a supported version of kubernetes")
+	cmd.Flags().StringVarP(&o.DownloadCmd, "download-cmd", "", "curl -L -o %s %s",
+		`The user defined command to download the necessary binary files. The first param '%s' is output path, the second param '%s', is the URL`)
+	cmd.Flags().StringVarP(&o.Artifact, "artifact", "a", "", "Path to a KubeKey artifact")


Suggest using Complete() and Validate() methods to check the user input.

24sama · 2022-09-23T02:19:12Z

pkg/binaries/kubernetes.go

@@ -145,3 +146,76 @@ func KubernetesArtifactBinariesDownload(manifest *common.ArtifactManifest, path,

 	return nil
 }
+
+// CriDownloadHTTP defines the kubernetes' binaries that need to be downloaded in advance and downloads them.
+func DockerDownloadHTTP(kubeConf *common.KubeConf, path, arch string, pipelineCache *cache.Cache) error {


I think DockerDownloadHTTP() and ContainerdDownloadHTTP() can be modified as one function, which is can receive variable files.KubeBinary{}.
For example:

func DownloadBinaries(kubeConf *common.KubeConf, pipelineCache *cache.Cache, binaries ...*files.KubeBinary) { }

right,I will optimize it,soon

24sama · 2022-09-23T02:38:16Z

pkg/container/containerd.go

+}
+
+func (d *MigrateSelfNodeTocontainerd) Execute(runtime connector.Runtime) error {
+	nodeName, _ := runtime.GetRunner().SudoCmd("hostname", true)


What happened if the command hostname reported an error?

I will optimize it,soon

24sama · 2022-09-23T02:39:50Z

pkg/binaries/tasks.go

+func (d *DockerDownload) Execute(runtime connector.Runtime) error {
+	cfg := d.KubeConf.Cluster
+
+	// var kubeVersion string


Remove useless codes.

I will optimize it,soon

24sama · 2022-09-23T06:16:15Z

pkg/pipelines/migrate_container.go

+		loaderType = common.AllInOne
+	}
+
+	fmt.Println("MigrateCriPipeline  called")


Remove this line.

I will optimize it,soon

24sama · 2022-09-23T06:16:55Z

pkg/pipelines/migrate_container.go

+	}
+
+	if err := MigrateCriPipeline(runtime); err != nil {
+		fmt.Println(err)


Remove this line.

I will optimize it,soon

24sama · 2022-09-23T06:17:44Z

pkg/pipelines/migrate_container.go

+	fmt.Println("MigrateCriPipeline  called")
+	runtime, err := common.NewKubeRuntime(loaderType, args)
+	if err != nil {
+		fmt.Println(err)


Remove this line, which will cause print error message twice.

yes,I will optimize it,soon

24sama · 2022-09-23T06:21:19Z

pkg/container/containerd.go

+	nodeName, _ := runtime.GetRunner().SudoCmd("hostname", true)
+	if _, err := runtime.GetRunner().SudoCmd(fmt.Sprintf("/usr/local/bin/kubectl cordon %s ", nodeName), true); err != nil {
+		return errors.Wrap(err, fmt.Sprintf("cordon the node: %s failed", nodeName))
+	}
+	if _, err := runtime.GetRunner().SudoCmd(fmt.Sprintf(
+		"/usr/local/bin/kubectl drain %s --delete-emptydir-data --ignore-daemonsets --timeout=2m --force", nodeName),
+		true); err != nil {
+		return errors.Wrap(err, fmt.Sprintf("drain the node: %s failed", nodeName))
+	}
+	// if _, err := runtime.GetRunner().SudoCmd("systemctl stop kubelet", false); err != nil {
+	// 	return errors.Wrap(errors.WithStack(err), fmt.Sprintf("stop kubelet failed: %s", nodeName))
+	// }
+	if err := UninstallContainerTasks(runtime, d.KubeAction); err != nil {
+		return errors.Wrap(errors.WithStack(err), fmt.Sprintf("Uninstall ContainerTasks failed: %s", nodeName))
+	}


I see these codes are duplicated with MigrateSelfNodeTocontainerd, so I suggest modifying them as a new task.
Just an advice, maybe after that the MigrateSelfNodeToDocker can become to:

func (d *MigrateSelfNodeToDocker) Execute(runtime connector.Runtime) error { CordonTask DrainTask uninstallTask MigrateTask RestartTask UnCordonTask }

thanks, a good advice, I will optimize it,soon

24sama · 2022-09-23T06:41:03Z

pkg/container/module.go

+	if p.KubeConf.Arg.Type == common.Docker && p.KubeConf.Arg.Role == common.Worker {
+		p.Tasks = MigrateToDocker(p)
+	} else if p.KubeConf.Arg.Type == common.Docker && p.KubeConf.Arg.Role == common.Master {
+		p.Tasks = MigrateMToDocker(p)
+	} else if p.KubeConf.Arg.Type == common.Conatinerd && p.KubeConf.Arg.Role == common.Worker {
+		p.Tasks = MigrateToContainerd(p)
+	} else if p.KubeConf.Arg.Type == common.Conatinerd && p.KubeConf.Arg.Role == common.Master {
+		p.Tasks = MigrateMToContainerd(p)
+	} else {
+		logger.Log.Fatalf("Unsupported container runtime: %s", strings.TrimSpace(p.KubeConf.Arg.Type))
+	}


What happened if the flag role support this --role all? Add a new module?
Maybe we just need to add a new prepare to determine if the node should be skipped.

var action task.Action switch type { case docker: action = new(MigrateSelfNodeToDocker) case contained: action = new(MigrateSelfNodeToContainerd) } migrate = &task.RemoteTask{ Name: "MigrateToDocker", Desc: "Migrate To Docker", Hosts: p.Runtime.GetHostsByRole(common.k8s), Prepare: new(RolePrepare), Action: action, Parallel: false, }

ok,I will optimize it,soon

24sama · 2022-09-28T02:09:41Z

pkg/container/containerd.go

+		}
+
+	}
+	if _, err := runtime.GetRunner().SudoCmd(fmt.Sprintf("/usr/local/bin/kubectl uncordon %s", nodeName), true); err != nil {


Why does this task need to execute twice uncordon commands?

24sama · 2022-09-28T02:22:48Z

cmd/ctl/cri/migrate_cri.go

+	cmd.Flags().StringVarP(&o.Role, "role", "", "worker", "Role to migrate")
+	cmd.Flags().StringVarP(&o.Type, "type", "", "docker", "Type to migrate")


It is recommended not to set the default value. And the description needs more detailed, for example, which roles can be specified, and which types can be specified.

24sama · 2022-09-28T02:24:07Z

cmd/ctl/cri/migrate_cri.go

+	if o.Role == "" {
+		return errors.New("node Role can not be empty")
+	}
+	if o.Type == "" {
+		return errors.New("cri Type can not be empty")
+	}


Need to check the user input value if it is valid, rather than only check if it is empty.

24sama · 2022-09-28T03:10:37Z

cmd/ctl/cri/migrate_cri.go

+
+func (o *MigrateCriOptions) Validate() error {
+	if o.Role == "" || (o.Role != common.Worker && o.Role != common.Master && o.Role != "all") {
+		return errors.New("node Role should be valid ")


if o.Role == "" { return errors.New("The role can not be empty") } if o.Role != common.Worker && o.Role != common.Master && o.Role != "all" { return errors.Errorf("role %s is invalid", o.Role) }

Signed-off-by: zhoutian <zhoutian@yunify.com>

24sama · 2022-09-28T04:57:49Z

/lgtm
/approve

ks-ci-bot · 2022-09-28T04:57:54Z

LGTM label has been added.

Git tree hash: 9df097d779cd142634bff844c6570feedc507a50

ks-ci-bot · 2022-09-28T04:57:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 24sama, zt1046656665

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [24sama]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ks-ci-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 22, 2022

zt1046656665 force-pushed the master branch from a7d36ce to 01ca3d1 Compare September 23, 2022 02:02

zt1046656665 force-pushed the master branch from 01ca3d1 to 89a941e Compare September 23, 2022 02:29

zt1046656665 force-pushed the master branch from 89a941e to ea40b6f Compare September 23, 2022 02:38

zhouqiu0103 reviewed Sep 23, 2022

View reviewed changes

24sama reviewed Sep 23, 2022

View reviewed changes

zt1046656665 force-pushed the master branch from ea40b6f to 61a0ce0 Compare September 27, 2022 06:26

24sama reviewed Sep 28, 2022

View reviewed changes

zt1046656665 force-pushed the master branch from 61a0ce0 to 72a6ad6 Compare September 28, 2022 03:01

24sama reviewed Sep 28, 2022

View reviewed changes

zt1046656665 force-pushed the master branch from 72a6ad6 to a856427 Compare September 28, 2022 04:29

add migrate_cri

0ea2a0d

Signed-off-by: zhoutian <zhoutian@yunify.com>

zt1046656665 force-pushed the master branch from a856427 to 0ea2a0d Compare September 28, 2022 04:55

ks-ci-bot assigned 24sama Sep 28, 2022

ks-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2022

ks-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2022

ks-ci-bot merged commit 68bd1dd into kubesphere:master Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add migrate_cri #1519

add migrate_cri #1519

zt1046656665 commented Sep 22, 2022

zhouqiu0103 commented Sep 23, 2022 •

edited

Loading

zt1046656665 commented Sep 23, 2022

zhouqiu0103 Sep 23, 2022

zhouqiu0103 Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama left a comment

24sama Sep 23, 2022

24sama Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 23, 2022

zt1046656665 Sep 23, 2022

24sama Sep 28, 2022

24sama Sep 28, 2022

24sama Sep 28, 2022

24sama Sep 28, 2022

24sama commented Sep 28, 2022

ks-ci-bot commented Sep 28, 2022

ks-ci-bot commented Sep 28, 2022

		cmd.Flags().StringVarP(&o.Role, "role", "", "worker", "Role to migrate")
		cmd.Flags().StringVarP(&o.Type, "type", "", "docker", "Type to migrate")

add migrate_cri #1519

add migrate_cri #1519

Conversation

zt1046656665 commented Sep 22, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Does this PR introduced a user-facing change?

Additional documentation, usage docs, etc.:

zhouqiu0103 commented Sep 23, 2022 • edited Loading

zt1046656665 commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

24sama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

24sama commented Sep 28, 2022

ks-ci-bot commented Sep 28, 2022

ks-ci-bot commented Sep 28, 2022

zhouqiu0103 commented Sep 23, 2022 •

edited

Loading